Skip to content

Conversation

@joshheald
Copy link
Contributor

@joshheald joshheald commented Aug 29, 2025

Part of: WOOMOB-1210
Merge after: #16061

Description

This PR adds mapping to convert between POS models and Persisted models. Only basic properties are covered, not relationships, which will be handled in a future PR.

Steps to reproduce

None, these are not used in the app yet, just check unit tests.


  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@joshheald joshheald added this to the 23.2 milestone Aug 29, 2025
@joshheald joshheald added type: task An internally driven task. status: feature-flagged Behind a feature flag. Milestone is not strongly held. feature: POS labels Aug 29, 2025
@joshheald joshheald changed the base branch from trunk to woomob-1210-local-catalog-persistence-classes August 29, 2025 17:49
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Aug 29, 2025

App Icon📲 You can test the changes from this Pull Request in WooCommerce iOS Prototype by scanning the QR code below to install the corresponding build.

App NameWooCommerce iOS Prototype
Build Numberpr16062-2159f0d
Version23.1
Bundle IDcom.automattic.alpha.woocommerce
Commit2159f0d
Installation URL57ljqnf3ku10g
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

@joshheald joshheald marked this pull request as ready for review August 29, 2025 18:13
@joshheald joshheald requested a review from jaclync August 29, 2025 18:18
public func toProductAttribute(siteID: Int64) -> ProductAttribute {
return ProductAttribute(
siteID: siteID,
attributeID: 0,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we decide to set the remote ID in a separate column (#16047 (comment)), this can be set. For now, do we want to set this with id ?? 0 like in the variation attribute?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in f822a09

The risk (which was already worse than it is now) is that if we call toProductAttribute on a global attribute which hasn't been saved, we'll get 0 instead of nil, which suggests a local attribute. But then only local attributes are properly handled so far anyway.

Comment on lines 12 to 29
let images: [ProductImage] = [
ProductImage(imageID: 100,
dateCreated: Date(timeIntervalSince1970: 1),
dateModified: nil,
src: "https://example.com/1.png",
name: "img1",
alt: "alt1"),
ProductImage(imageID: 101,
dateCreated: Date(timeIntervalSince1970: 2),
dateModified: Date(timeIntervalSince1970: 3),
src: "https://example.com/2.png",
name: "img2",
alt: nil)
]
let attributes: [ProductAttribute] = [
ProductAttribute(siteID: siteID, attributeID: 0, name: "Color", position: 0, visible: true, variation: true, options: ["Red", "Blue"]),
ProductAttribute(siteID: siteID, attributeID: 0, name: "Size", position: 1, visible: false, variation: false, options: ["S", "M"])
]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super nit: since these aren't being asserted on in this test case, maybe can pass empty arrays for simplicity.

struct PersistedProductConversionsTests {

@Test("PersistedProduct init(from:) maps all POSProduct fields")
func test_product_init_from_posProduct_maps_all_fields() throws {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super nit: with Swift Testing, we probably don't need the test_ prefix in the function name as each test case is already marked with @Test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll update them. TBH I think we could also leave behind the snake case, if we use descriptive names as arguments to @Test – they display much nicer anyway, at least within Xcode. I'm not sure whether CI pays any attention to them though.

@joshheald joshheald merged commit 64b030a into woomob-1210-local-catalog-persistence-classes Sep 1, 2025
14 checks passed
@joshheald joshheald deleted the woomob-1210-basic-mapping-from-POS-entities-to-persisted branch September 1, 2025 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature: POS status: feature-flagged Behind a feature flag. Milestone is not strongly held. type: task An internally driven task.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants